Skip to content

Fixed a couple of (very rare) race conditions #2112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 15, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Fixed a couple of theoretical race conditions (they are detected by the race condition detector, but they are very unlikely to happen in a real-world scenario).

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change, and is titled accordingly?

No

Other information

cmaglie added 2 commits March 15, 2023 11:27
The go-routine was spawned before the process was started. This means
that p.Kill may read p.cmd.Process before it is written in p.Run.
@cmaglie cmaglie self-assigned this Mar 15, 2023
@cmaglie cmaglie added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Mar 15, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 25.00% and project coverage change: +0.11 🎉

Comparison is base (245e84c) 35.05% compared to head (2285176) 35.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
+ Coverage   35.05%   35.17%   +0.11%     
==========================================
  Files         232      232              
  Lines       20480    20498      +18     
==========================================
+ Hits         7180     7210      +30     
+ Misses      12455    12444      -11     
+ Partials      845      844       -1     
Flag Coverage Δ
unit 35.17% <25.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
executils/process.go 78.46% <25.00%> (-4.08%) ⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cmaglie cmaglie merged commit 5110f0b into arduino:master Mar 16, 2023
@cmaglie cmaglie deleted the fix_race_conditions branch March 16, 2023 14:10
kittaakos pushed a commit to kittaakos/arduino-cli that referenced this pull request Mar 21, 2023
* Fixed concurrent write to variables `err` and `n`

* Fix concurrent access to p.cmd.Process

The go-routine was spawned before the process was started. This means
that p.Kill may read p.cmd.Process before it is written in p.Run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants